[v2] Perf: Use one atom subscription per Field - #2358
Conversation
Each React Field subscribed to the same atom once for value and again for meta. The field atom already preserves state identity when neither changes, so one whole-state subscription has the same render behavior. Co-Authored-By: OpenAI Codex <noreply@openai.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe field subscription now observes the complete field state through one subscription. The field-context test description and expected active subscription count reflect this change. ChangesField subscription update
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized performance change reduces each field's atom subscriptions from two to one while preserving the stated render behavior; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
You mention performance for the PR. Do you have some rough numbers for it? Memory / runtime? |
|
@LeCarbonator all the perf is just from going from 2 to 1 I ran a focused benchmark using the exact before/after hooks with production React 19.2.8. Each variant ran in a fresh process 10 times, alternating order; results are medians. 1,000 mounted fields
The harness mounts non-array fields backed by individual TanStack Store atoms, measures synchronous mount/update work, and measures retained heap after forced GC. This isolates subscription overhead rather than measuring a complete application, so the absolute timings are directional. The consistent result is half the subscriptions and callbacks, with roughly 3.2 KiB less retained heap per mounted field. |
|
Sorry, don't have a ton of cycles here to respond more in depth, but I'd attempted this in v1 and benchmarks showed MASSIVE improvements then (like 10x), but the UX of 1000 mounted fields slowed to a crawl for reasons I couldn't identify. Might be helpful for us to all sit down and discuss and on our end manually test. Not saying that this is a bad PR or approach, just flagging that benchmarks for this has been directionally wrong before. |
|
Let sol on ultra try to figure out what you were referring to, obviously feel free to review/close whatever. Model used: OpenAI Codex — GPT-5.6 Sol Codex investigated the relevant v1 history and benchmarked the exact base and PR commits in production Chromium. The closest v1 match is PR #2036. That implementation replaced seven subscriptions with one selector that created a new seven-property object and used a custom comparator. It was also mixed with substantial core store and notification changes, so the historical results do not isolate subscription consolidation as the cause. Its “1,000-field” benchmarks actually rendered 100 fields in jsdom and did not measure browser rendering or memory. V2 has a different design. Before this PR, a normal field subscribes separately to if (prev?.meta === meta && prev.value === value) {
return prev
}Therefore, the rerender conditions are equivalent: The PR does not create a projected object, add a custom comparator, or change core notification behavior. Real-browser confirmation with 1,000 controlled fields and 60 fresh contexts per variant found:
|
|
@scttcper I'm an idiot and I wasted your tokens. I just reviewed the diff quickly. For some reason I'd gotten a LOT wrong in my mind about what this PR was and assumed it was doing something more closely related to splitting one store into many for modularity. Frankly I don't know how I got things this mixed up. So sorry. This PR LGTM broadly, but might have weird consequences with Compiler. So long as it works well there we should merge. Sorry again, that's embarrassing. |
|
@crutchcorn work pays for it 🤷 |
|
I think this should be compiler safe. Don't see how changing it from two to one could have consequences for it. I wish I had better Wi-Fi to actually try this, but that'll have to wait until next monday. |
|
View your CI Pipeline Execution ↗ for commit 45ed204
☁️ Nx Cloud last updated this comment at |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #2358 +/- ##
========================================
Coverage ? 92.91%
========================================
Files ? 14
Lines ? 240
Branches ? 19
========================================
Hits ? 223
Misses ? 16
Partials ? 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Each React
Fieldsubscribed to the same atom twice, once for value and once for meta. The field atom already preserves state identity when neither changes, so use one whole-state subscription instead.This cuts active subscriptions from two to one per field with the same render behavior.
Summary by CodeRabbit
Bug Fixes
Tests